Issue/pool reporting #8293

wouterdb · 2024-10-30T13:39:12Z

Description

Update database statistics reporting

towards the logs
towards the status enpoint
towards influxdb

closes #8228

TODO post merge:

inform FE that we broke the API
update grafana dashboard on grafana hub

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Type annotations are present
Code is clear and sufficiently documented
No (preventable) type errors (check using make mypy or make mypy-diff)
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

wouterdb · 2024-10-30T13:39:50Z

mypy-baseline.txt

@@ -101,7 +101,7 @@ src/inmanta/ast/type.py:0: error: No overload variant of "reversed" matches argu
 src/inmanta/ast/type.py:0: note: Possible overload variants:
 src/inmanta/ast/type.py:0: note: def [_T] __new__(cls, Reversible[_T], /) -> reversed[_T]
 src/inmanta/ast/type.py:0: note: def [_T] __new__(cls, SupportsLenAndGetItem[_T], /) -> reversed[_T]
-src/inmanta/server/services/databaseservice.py:0: error: Incompatible return value type (got "dict[str, object]", expected "dict[str, BaseModel | UUID | bool | int | float | datetime | str | Sequence[BaseModel | UUID | bool | int | float | datetime | str] | Mapping[str, BaseModel | UUID | bool | int | float | datetime | str]]")  [return-value]
+src/inmanta/server/services/metricservice.py:0: error: Incompatible types in assignment (expression has type "float", variable has type "int")  [assignment]


pre-existing issue, moved

wouterdb · 2024-10-30T13:40:14Z

mypy-baseline.txt

@@ -1101,6 +1103,7 @@ src/inmanta/server/services/orchestrationservice.py:0: error: Incompatible types
 src/inmanta/server/services/orchestrationservice.py:0: error: Incompatible types in assignment (expression has type "set[str | None]", variable has type "set[str]")  [assignment]
 src/inmanta/server/services/orchestrationservice.py:0: error: Argument "agents" to "_trigger_auto_deploy" of "OrchestrationService" has incompatible type "Collection[str]"; expected "Sequence[str] | None"  [arg-type]
 src/inmanta/server/services/orchestrationservice.py:0: error: Incompatible return value type (got "ReturnValueWithMeta[Sequence[DesiredStateVersion]]", expected "ReturnValue[list[DesiredStateVersion]]")  [return-value]
+src/inmanta/server/agentmanager.py:0: error: Value of type "dict[str, Any] | None" is not indexable  [index]


decoding of api results, difficult to avoid

wouterdb · 2024-10-30T13:40:39Z

src/inmanta/agent/agent_new.py

+        self._db_monitor = DatabaseMonitor(
+            data.Resource._connection_pool,
+            opt.db_name.get(),
+            opt.db_host.get(),
+            "scheduler",
+            str(self.environment),
+        )
+        self._db_monitor.start()


Scheduler gets DB monitor as well

wouterdb · 2024-10-30T13:41:03Z

src/inmanta/agent/agent_new.py

+    async def get_db_status(self) -> DataBaseReport:
+        if self._db_monitor is None:
+            return DataBaseReport(
+                connected=False,
+                database="",
+                host="",
+                max_pool=0,
+                open_connections=0,
+                free_connections=0,
+                pool_exhaustion_count=0,
+            )
+        return await self._db_monitor.get_status()


new status endpoint on the scheduler to get database status

wouterdb · 2024-10-30T13:41:19Z

src/inmanta/app.py

+        metrics_reporter = MetricsService()
+        metrics_reporter.start_metric_reporters()


scheduler will now also report to influxdb

wouterdb · 2024-10-30T13:41:33Z

src/inmanta/data/model.py

@@ -89,7 +89,7 @@ class SliceStatus(BaseModel):
    """

    name: str
-    status: dict[str, ArgumentTypes]
+    status: Mapping[str, ArgumentTypes | Mapping[str, ArgumentTypes]]


This breaks the FE, but allow nicer presentation

wouterdb · 2024-10-30T13:41:55Z

src/inmanta/server/__init__.py

@@ -19,7 +19,7 @@
 # flake8: noqa: F401

 SLICE_SERVER = "core.server"
-SLICE_AGENT_MANAGER = "core.agentmanager"
+SLICE_AGENT_MANAGER = "core.scheduler_manager"


rename for FE, hopefully OK

wouterdb · 2024-10-30T13:43:09Z

src/inmanta/server/agentmanager.py

@@ -168,12 +169,63 @@ def __init__(self, closesessionsonstart: bool = True, fact_back_off: Optional[in

        self.closesessionsonstart: bool = closesessionsonstart

-    async def get_status(self) -> dict[str, ArgumentTypes]:
-        return {
+    async def get_status(self) -> Mapping[str, ArgumentTypes | Mapping[str, ArgumentTypes]]:


This method is very tricky, as it has to return withing the timeout of the status endpoint.

I chose to make it report what it has, even if that makes the format somewhat inconsistent.
(e.g. if the DB is down, the environments will be reported by UUID instead of name)

wouterdb · 2024-10-30T13:43:36Z

src/inmanta/server/services/databaseservice.py

+    from inmanta.server import agentmanager
+
+
+class DatabaseMonitor:


Factored out!

wouterdb · 2024-10-30T13:43:54Z

src/inmanta/server/services/databaseservice.py

+
+    def start_monitor(self) -> None:
+        """Attach to monitoring system"""
+        suffix = f",component={self.component}"


nasty way of adding tags, but it works

wouterdb · 2024-10-30T13:44:10Z

src/inmanta/server/services/databaseservice.py

+        gauge(
+            "db.pool_exhaustion_count" + suffix,
+            CallbackGauge(callback=lambda: self._db_pool_watcher._exhausted_pool_events_count),
+        )


this is a new metric

Hugo-Inmanta · 2024-10-30T14:00:28Z

src/inmanta/data/model.py

+        if other.host != self.host:
+            return NotImplemented
+        return DataBaseReport(
+            connected=self.connected,


What about different connected values?

good question, I should probably and them

Hugo-Inmanta · 2024-10-30T14:39:21Z

src/inmanta/server/services/databaseservice.py

+        suffix = f",component={self.component}"
+        if self.environment:
+            suffix += f",environment={self.environment}"


suffix could be factored out, e.g. into a property, since it's also used in start_monitor.

tests/test_influxdbreporting.py

stubs/pyformance/__init__.pyi

src/inmanta/server/agentmanager.py

Co-authored-by: Hugo-Inmanta <[email protected]>

…e into issue/pool_reporting

Hugo-Inmanta · 2024-10-31T08:59:23Z

src/inmanta/server/services/databaseservice.py

@@ -74,44 +81,43 @@ async def get_status(self) -> DataBaseReport:
            pool_exhaustion_count=self._db_pool_watcher._exhausted_pool_events_count,
        )

+    def _add_guage(self, name: str, the_gauge: CallbackGauge) -> None:


Suggested change

def _add_guage(self, name: str, the_gauge: CallbackGauge) -> None:

def _add_gauge(self, name: str, the_gauge: CallbackGauge) -> None:

Hugo-Inmanta · 2024-10-31T08:59:34Z

src/inmanta/server/services/databaseservice.py

+        if self.environment:
+            self.infuxdb_suffix += f",environment={self.environment}"
+
+        self.registered_guages: list[str] = []


Suggested change

self.registered_guages: list[str] = []

self.registered_gauges: list[str] = []

src/inmanta/server/agentmanager.py

sanderr

I'm only vaguelly familiar with all of this tbh.

sanderr · 2024-10-31T15:17:35Z

changelogs/unreleased/8228-database_pool_report.yml

+change-type: minor
+destination-branches: [master]
+sections:
+  minor-improvement: "{{description}}"


Does this deserve a change entry, considering that the new scheduler has not been publicly released? Or has it?

It is a user visible change, but perhaps I should add more detail?

I mean the entire scheduler is new, so I would assume that to be one change entry rather than a separate one for each prt it affects.

src/inmanta/server/agentmanager.py

sanderr · 2024-10-31T15:35:23Z

src/inmanta/util/__init__.py

@@ -392,6 +392,25 @@ def action_function() -> None:
        self._scheduled[task_spec] = handle
        return task_spec

+    def schedule(


What does this method offer? I do prefer the name, but I feel like it's just a layer on top of add_action that makes a tiny streamline of the interface for one type of schedules, at the cost of dropping support for the other. So now we need to call either schedule() or add_action() depending on the use case.

It is indeed purely convenience. The ServerSlice has a similar method, with similar usecase.

I can remove it if you don't think it should be here.

To me it seems like it adds more confusion than anything else. If I see a method schedule I'd assume that to be the main interface, but it isn't because it only supports one of the two schedule types.

So I prefer it without, but if you feel it's useful I won't stand in the way.

sanderr · 2024-10-31T15:40:00Z

src/inmanta/server/protocol.py

@@ -399,7 +390,7 @@ def get_extension_statuses(cls, slices: list["ServerSlice"]) -> list[ExtensionSt
                result[ext_status.name] = ext_status
        return list(result.values())

-    async def get_status(self) -> dict[str, ArgumentTypes]:
+    async def get_status(self) -> Mapping[str, ArgumentTypes | Mapping[str, ArgumentTypes]]:


Why change all these from dict to Mapping?

Due to invariance of dict, the Union is a bit hard to use otherwise and it is inteded to be mapping all along

But Mapping is compatible with dict, so you can just return whatever you're actually putting in there, no?

I'm not necessarily against it though.

wouterdb added 7 commits October 25, 2024 13:36

temp

6bc1daf

All bits and pieces

66670dc

added DB server refactor

4966ef2

Merge remote-tracking branch 'origin/master' into issue/pool_reporting

d66600d

temp

98e12fb

Added reporting

74ee0f9

added changelog

06cb13b

wouterdb requested review from sanderr and Hugo-Inmanta October 30, 2024 13:39

wouterdb commented Oct 30, 2024

View reviewed changes

wouterdb added 2 commits October 30, 2024 15:08

cleanup

188e67f

added comments

02095e0

Hugo-Inmanta requested changes Oct 30, 2024

View reviewed changes

wouterdb and others added 2 commits October 31, 2024 08:42

Update tests/test_influxdbreporting.py

cc1afe3

Co-authored-by: Hugo-Inmanta <[email protected]>

Fixes

2f38e1a

wouterdb requested a review from Hugo-Inmanta October 31, 2024 08:42

Merge branch 'issue/pool_reporting' of github.com:inmanta/inmanta-cor…

4fbff56

…e into issue/pool_reporting

Hugo-Inmanta requested changes Oct 31, 2024

View reviewed changes

Fix typo

9d72fd3

wouterdb requested a review from Hugo-Inmanta October 31, 2024 09:15

Hugo-Inmanta approved these changes Oct 31, 2024

View reviewed changes

matborowczyk mentioned this pull request Oct 31, 2024

Add the ability to display nested properties in the server status view inmanta/web-console#6018

Open

sanderr requested changes Oct 31, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into issue/pool_reporting

1861b0a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/pool reporting #8293

Issue/pool reporting #8293

wouterdb commented Oct 30, 2024 •

edited

Loading

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

wouterdb Oct 30, 2024

Hugo-Inmanta Oct 30, 2024

wouterdb Oct 31, 2024

Hugo-Inmanta Oct 30, 2024

Hugo-Inmanta Oct 31, 2024

Hugo-Inmanta Oct 31, 2024

sanderr left a comment

sanderr Oct 31, 2024

wouterdb Oct 31, 2024

sanderr Oct 31, 2024

sanderr Oct 31, 2024

wouterdb Oct 31, 2024

sanderr Oct 31, 2024

sanderr Oct 31, 2024

wouterdb Oct 31, 2024

sanderr Oct 31, 2024

		metrics_reporter = MetricsService()
		metrics_reporter.start_metric_reporters()

		from inmanta.server import agentmanager


		class DatabaseMonitor:

	def _add_guage(self, name: str, the_gauge: CallbackGauge) -> None:
	def _add_gauge(self, name: str, the_gauge: CallbackGauge) -> None:

	self.registered_guages: list[str] = []
	self.registered_gauges: list[str] = []

Issue/pool reporting #8293

Are you sure you want to change the base?

Issue/pool reporting #8293

Conversation

wouterdb commented Oct 30, 2024 • edited Loading

Description

TODO post merge:

Self Check:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterdb commented Oct 30, 2024 •

edited

Loading